Repository navigation
Conversation
…r in the content pane Provider cards no longer force a white tile behind every icon in dark mode. Near-black strokes in inline SVGs are rendered with currentColor, and a near-black default icon color is left to the surrounding text color. The icon picker now opens as a FullScreenPanel over the content area instead of a full-window dialog, so the sidebar and window controls stay in place. The upload hint is shown as text instead of a long tooltip.
farion1231
left a comment
There was a problem hiding this comment.
Thanks for this — moving the icon picker into the content pane so the back button stops colliding with the window controls is a nice fix, and the merge tree passes typecheck and the full unit suite. I checked every built-in icon (96 inline SVGs, 45 URL icons) plus the picker flow against main and found three regressions:
[P2] Esc in the icon picker now closes the whole provider editor
Both the editor and the new picker are FullScreenPanels, and each one registers its own window keydown listener (FullScreenPanel.tsx#L123). stopPropagation() does not stop other listeners on the same target, and the outer panel registered first, so it runs first. Opening the picker leaves focus on the icon button behind the panel, so pressing Esc reaches the outer handlePanelClose (EditProviderDialog.tsx#L129): the edit page closes and unsaved edits are lost (on the Add page it jumps back to the preset step). I reproduced it with BasicFormFields inside a FullScreenPanel: on this branch the outer onClose fires once; on main the Radix dialog moves focus inside and consumes Esc, so the outer panel is not touched.
Fix: let only the topmost panel handle Esc (e.g. a small open-panel stack in FullScreenPanel), or have the picker panel handle Esc in the capture phase with stopImmediatePropagation(), and move focus into the panel when it opens.
[P2] adaptSvgToTheme breaks multi-color and tile icons, in light mode too
index.ts#L228-L260 swaps dark fills for currentColor, but ProviderIcon sets style.color to the icon's defaultColor, so the swapped parts get the brand color:
- Hugging Face (used by presets): the eyes and mouth (
#3A3B45) become#FFD21E, the same yellow as the face, so the face has no features in either theme. LionCC is affected the same way. - OpenClaw (the sidebar app icon): the eyes (
#050810) become#ff4f40, the same red as the body. lum < 60on raw sRGB counts saturated reds as near-black: Huawei (#C7000B, and its#FF0000default color) renders entirely in the text color, and Stability's red part does too.- Tile icons aren't excluded: SSAI Code's background
<rect fill="#08080e">becomescurrentColor, so in dark mode it turns into a light-gray tile with pale glyphs.
Fix: only recolor single-ink icons (every painted color is near-black). Skip anything with other non-gray colors, skip isTileIcon, and use a real near-black test (e.g. max channel ≤ ~0x40) instead of weighted luminance.
[P2] Without the white tile, icons that can't be recolored become invisible on dark cards
ProviderCard.tsx#L329 removes the background for every icon, but several have no themable path:
- Raster icons with a transparent background and dark ink, e.g.
atlascloud(used by presets: a solid black "A" on transparent). - Custom uploaded icons: transparency is kept when the file is encoded, and they render through
<img>, so a black logo on a transparent background disappears. - Novita and OpenCode: the
fill="white"inside their<clipPath>/<mask>defs counts as "light ink", soadaptSvgToThemeskips them and#000000/#211E1Estay as they are.
Fix: keep a contrasting background in dark mode for URL/custom icons (and for any SVG the adapter leaves untouched), and ignore paints inside <defs>, <clipPath> and <mask> when deciding whether to adapt.
Once these three are fixed I'm happy to take another look quickly.
- Esc closes only the most recently opened FullScreenPanel, so the icon picker no longer takes the provider editor down with it. - Only near-black paints (all channels low) are recolored; masks, clip paths and defs are ignored, tile icons are skipped, and recolored icons no longer get the brand color applied over them. - Keep a light backing in dark mode for raster/uploaded icons and SVGs that still carry near-black ink after adaptation.
|
Thanks for the thorough check. Pushed a follow-up commit:
🤖 Addressed by Claude Code |
farion1231
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up. I re-checked ab99c57 merged into current main (typecheck and the full unit suite pass) and rendered every built-in icon again in light and dark mode against main. Esc now closes only the icon picker, saturated reds (Huawei, Stability) and tile icons are left alone, and raster / uploaded icons as well as Novita and OpenCode now read fine on the dark card. One case from the last round is still off:
[P2] Hugging Face loses its eyes and mouth in dark mode
Its face details are #3A3B45 drawn on a #FFD21E face. They are now swapped to currentColor, which on a dark card resolves to the light text color (#e4e4e7). Light-on-yellow is about 1.1:1 contrast, so at card size the eyes and mouth practically disappear (light mode is fine). Hugging Face is a visible preset in six apps, and the icon picker grid shows the same thing. LionCC has the same pattern (#111111 linework on #F9DA3C).
- Recolor logic:
cc-switch/src/icons/extracted/index.ts
Lines 260 to 284 in ab99c57
- The new test asserts this recolor:
cc-switch/tests/icons/themeAdapt.test.ts
Lines 21 to 25 in ab99c57
One way to handle it: when near-black paint sits next to other non-near-black fills, treat the icon like the "has light ink" case and leave it as-is, so needsLightBacking gives it the light backing instead (something along these lines, untested):
const skip =
paints.some((c) => c.trim().toLowerCase() === "currentcolor") ||
paints.some(isLightInk) ||
// dark details on a coloured fill (Hugging Face, LionCC): keep the original colors and back it instead
(paints.some(isDarkInk) && paints.some((c) => c.startsWith("#") && !isDarkInk(c)));nit: split(MASK_BLOCK) re-inserts the tag name into the SVG
MASK_BLOCK has two capture groups, so split yields [text, block, tagName, text, ...], and the .join("") puts the tag name back as bare text, e.g. ...</linearGradient></defs>defs<path .... That affects 21 built-in SVGs. It is not rendered, but it also makes adapted true for icons where nothing was recolored (Azure, Gemini, Meta, Qwen, ...), so their defaultColor gets dropped. Skipping the tag-name parts fixes it:
.split(MASK_BLOCK)
.filter((_, i) => i % 3 !== 2)
.map((part, i) => (i % 2 === 0 ? part.replace(PAINT_ATTR, ...) : part))Happy to re-check quickly once the Hugging Face case is addressed.
Multi-color icons keep their near-black details (Hugging Face eyes, LionCC linework), which read fine on the colored shapes. Inline SVGs no longer get a light backing; only raster/URL icons do.
|
Good catch. 🤖 Addressed by Claude Code |
|
@farion1231 the follow-up from the second review is pushed (multi-color icons are left untouched, only single-ink icons are recolored). Could you take another look when you have time? |
Summary
currentColor; a near-black default icon color is left to the surrounding text color. Self-contained icons (tile icons, icons with their own light ink) are untouched.FullScreenPanelover the content area instead of a full-window dialog, so the sidebar and window controls stay in place and the back button no longer overlaps the traffic lights.Test plan
pnpm typecheck, prettierUsageDashboardandrememberedChoicetests fail onmainas well, unrelated to this change.